Add a notion of "physical" sysroot, use for remote writing

Using `${sysroot}` to mean the physical storage root: We don't want to write to
`${sysroot}/etc/ostree/remotes.d`, since nothing will read it, and really
`${sysroot}` should just have `/ostree` (ideally). Today the Anaconda rpmostree
code ends up writing there. Fix this by adding a notion of "physical" sysroot.
We determine whether the path is physical by checking for `/sysroot`, which
exists in deployment roots (and there shouldn't be a `${sysroot}/sysroot`).

In order to unit test this, I added a `--sysroot` argument to `remote add`.
However, doing this better would require reworking the command line parsing for
the `remote` argument to support specifying `--repo` or `--sysroot`, and I
didn't quite want to do that yet in this patch.

Closes: https://github.com/ostreedev/ostree/issues/892
Closes: #896
Approved by: jlebon

diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h
index 015a9a8bd83f2e0010fa5a9666c43e9f908dffab..6a7e9ee8a14ccf5b5ee69cca049384ad38be6078 100644 (file)
--- a/src/libostree/ostree-repo-private.h
+++ b/src/libostree/ostree-repo-private.h
@@ -93,6 +93,7 @@ struct OstreeRepo {
   int objects_dir_fd;
   int uncompressed_objects_dir_fd;
   GFile *sysroot_dir;
+  GWeakRef sysroot; /* Weak to avoid a circular ref; see also `is_system` */
   char *remotes_config_dir;
 
   GHashTable *txn_refs;

diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c
index cbbaec9b0aaf99f7d5d35f87fd79410d4a851d2d..4ad112df226b9db59f6845e0ea51ee403326eef4 100644 (file)
--- a/src/libostree/ostree-repo.c
+++ b/src/libostree/ostree-repo.c
@@ -32,6 +32,7 @@
 #include <glnx-console.h>
 
 #include "ostree-core-private.h"
+#include "ostree-sysroot-private.h"
 #include "ostree-remote-private.h"
 #include "ostree-repo-private.h"
 #include "ostree-repo-file.h"
@@ -447,6 +448,7 @@ ostree_repo_finalize (GObject *object)
   if (self->uncompressed_objects_dir_fd != -1)
     (void) close (self->uncompressed_objects_dir_fd);
   g_clear_object (&self->sysroot_dir);
+  g_weak_ref_clear (&self->sysroot);
   g_free (self->remotes_config_dir);
 
   if (self->loose_object_devino_hash)
@@ -525,10 +527,6 @@ ostree_repo_constructed (GObject *object)
 
   g_assert (self->repodir != NULL);
 
-  /* Ensure the "sysroot-path" property is set. */
-  if (self->sysroot_dir == NULL)
-    self->sysroot_dir = g_object_ref (_ostree_get_default_sysroot_path ());
-
   G_OBJECT_CLASS (ostree_repo_parent_class)->constructed (object);
 }
 
@@ -706,8 +704,6 @@ ostree_repo_new_default (void)
 gboolean
 ostree_repo_is_system (OstreeRepo   *repo)
 {
-  g_autoptr(GFile) default_repo_path = NULL;
-
   g_return_val_if_fail (OSTREE_IS_REPO (repo), FALSE);
 
   /* If we were created via ostree_sysroot_get_repo(), we know the answer is yes
@@ -716,8 +712,11 @@ ostree_repo_is_system (OstreeRepo   *repo)
   if (repo->is_system)
     return TRUE;
 
-  default_repo_path = get_default_repo_path (repo->sysroot_dir);
+  /* No sysroot_dir set?  Not a system repo then. */
+  if (!repo->sysroot_dir)
+    return FALSE;
 
+  g_autoptr(GFile) default_repo_path = get_default_repo_path (repo->sysroot_dir);
   return g_file_equal (repo->repodir, default_repo_path);
 }
 
@@ -894,23 +893,25 @@ impl_repo_remote_add (OstreeRepo     *self,
 
   remote = ostree_remote_new (name);
 
-  /* The OstreeRepo maintains its own internal system root path,
-   * so we need to not only check if a "sysroot" argument was given
-   * but also whether it's actually different from OstreeRepo's.
-   *
-   * XXX Having API regret about the "sysroot" argument now.
+  /* If a sysroot was provided, use it. Otherwise, see if this repo has a ref to
+   * a sysroot (and it's physical).
    */
-  gboolean different_sysroot = FALSE;
-  if (sysroot != NULL)
-    different_sysroot = !g_file_equal (sysroot, self->sysroot_dir);
+  g_autoptr(OstreeSysroot) sysroot_ref = NULL;
+  if (sysroot == NULL)
+    {
+      sysroot_ref = (OstreeSysroot*)g_weak_ref_get (&self->sysroot);
+      /* Only write to /etc/ostree/remotes.d if we are pointed at a deployment */
+      if (sysroot_ref != NULL && !sysroot_ref->is_physical)
+        sysroot = ostree_sysroot_get_path (sysroot_ref);
+    }
+  /* For backwards compat, also fall back to the sysroot-path variable */
+  if (sysroot == NULL && sysroot_ref == NULL)
+    sysroot = self->sysroot_dir;
 
-  if (different_sysroot || ostree_repo_is_system (self))
+  if (sysroot != NULL)
     {
       g_autoptr(GError) local_error = NULL;
 
-      if (sysroot == NULL)
-        sysroot = self->sysroot_dir;
-
       g_autoptr(GFile) etc_ostree_remotes_d = g_file_resolve_relative_path (sysroot, SYSCONF_REMOTES);
       if (!g_file_make_directory_with_parents (etc_ostree_remotes_d,
                                                cancellable, &local_error))

diff --git a/src/libostree/ostree-sysroot-private.h b/src/libostree/ostree-sysroot-private.h
index 14ee5cad965c4c8730272858083d0fe66b9d85c2..82abc8e77fee240090ddaf8d2e9cd54fd02fd65f 100644 (file)
--- a/src/libostree/ostree-sysroot-private.h
+++ b/src/libostree/ostree-sysroot-private.h
@@ -48,7 +48,8 @@ struct OstreeSysroot {
   GLnxLockFile lock;
 
   gboolean loaded;
-  
+
+  gboolean is_physical; /* TRUE if we're pointed at physical storage root and not a deployment */
   GPtrArray *deployments;
   int bootversion;
   int subbootversion;

diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c
index 86aa8ce36a3e610f1e59416891fdd3e397819489..b8f494bffdc68c0c26e1b9c583fa49dde76cc4a9 100644 (file)
--- a/src/libostree/ostree-sysroot.c
+++ b/src/libostree/ostree-sysroot.c
@@ -135,6 +135,8 @@ ostree_sysroot_constructed (GObject *object)
   repo_path = g_file_resolve_relative_path (self->path, "ostree/repo");
   self->repo = ostree_repo_new_for_sysroot_path (repo_path, self->path);
   self->repo->is_system = TRUE;
+  /* Hold a weak ref for the remote-add handling */
+  g_weak_ref_init (&self->repo->sysroot, object);
 
   G_OBJECT_CLASS (ostree_sysroot_parent_class)->constructed (object);
 }
@@ -813,6 +815,26 @@ ostree_sysroot_load_if_changed (OstreeSysroot  *self,
                                cancellable, error))
     return FALSE;
 
+  /* Determine whether we're "physical" or not, the first time we initialize */
+  if (!self->loaded)
+    {
+      /* If we have a booted deployment, the sysroot is / and we're definitely
+       * not physical.
+       */
+      if (self->booted_deployment)
+        self->is_physical = FALSE;  /* (the default, but explicit for clarity) */
+      /* Otherwise - check for /sysroot which should only exist in a deployment,
+       * not in ${sysroot} (a metavariable for the real physical root).
+       */
+      else if (fstatat (self->sysroot_fd, "sysroot", &stbuf, 0) < 0)
+        {
+          if (errno != ENOENT)
+            return glnx_throw_errno_prefix (error, "fstatat");
+          self->is_physical = TRUE;
+        }
+      /* Otherwise, the default is FALSE */
+    }
+
   self->bootversion = bootversion;
   self->subbootversion = subbootversion;
   self->deployments = deployments;

diff --git a/src/ostree/ot-remote-builtin-add.c b/src/ostree/ot-remote-builtin-add.c
index 3e3aeda9784e3cf3806740bdcab3975cdce21aa0..9639b4a5588e16cc6dd4718f9e288ec098f49850 100644 (file)
--- a/src/ostree/ot-remote-builtin-add.c
+++ b/src/ostree/ot-remote-builtin-add.c
@@ -31,6 +31,7 @@ static gboolean opt_no_gpg_verify;
 static gboolean opt_if_not_exists;
 static char *opt_gpg_import;
 static char *opt_contenturl;
+static char *opt_sysroot;
 
 static GOptionEntry option_entries[] = {
   { "set", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_set, "Set config option KEY=VALUE for remote", "KEY=VALUE" },
@@ -38,6 +39,7 @@ static GOptionEntry option_entries[] = {
   { "if-not-exists", 0, 0, G_OPTION_ARG_NONE, &opt_if_not_exists, "Do nothing if the provided remote exists", NULL },
   { "gpg-import", 0, 0, G_OPTION_ARG_FILENAME, &opt_gpg_import, "Import GPG key from FILE", "FILE" },
   { "contenturl", 0, 0, G_OPTION_ARG_STRING, &opt_contenturl, "Use URL when fetching content", "URL" },
+  { "sysroot", 0, 0, G_OPTION_ARG_FILENAME, &opt_sysroot, "Use sysroot at PATH (overrides --repo)", "PATH" },
   { NULL }
 };
 
@@ -51,6 +53,7 @@ ot_remote_builtin_add (int argc, char **argv, GCancellable *cancellable, GError
   char **iter;
   g_autoptr(GVariantBuilder) optbuilder = NULL;
   g_autoptr(GVariant) options = NULL;
+  g_autoptr(OstreeSysroot) sysroot = NULL;
   gboolean ret = FALSE;
 
   context = g_option_context_new ("NAME [metalink=|mirrorlist=]URL [BRANCH...] - Add a remote repository");
@@ -59,6 +62,20 @@ ot_remote_builtin_add (int argc, char **argv, GCancellable *cancellable, GError
                                     OSTREE_BUILTIN_FLAG_NONE, &repo, cancellable, error))
     goto out;
 
+  /* As a special case, we can take a --sysroot argument. Currently we also
+   * require --repo because fixing that needs more cmdline rework.
+   */
+  if (opt_sysroot)
+    {
+      g_clear_object (&repo);
+      g_autoptr(GFile) sysroot_path = g_file_new_for_path (opt_sysroot);
+      sysroot = ostree_sysroot_new (sysroot_path);
+      if (!ostree_sysroot_load (sysroot, cancellable, error))
+        goto out;
+      if (!ostree_sysroot_get_repo (sysroot, &repo, cancellable, error))
+        goto out;
+    }
+
   if (argc < 3)
     {
       ot_util_usage_error (context, "NAME and URL must be specified", error);

diff --git a/tests/admin-test.sh b/tests/admin-test.sh
index cc06fe6f0c73dbd3383939bad1820c8ab91eaa5a..7182e60bd81ea53bac4f6851c72f90b10161670a 100644 (file)
--- a/tests/admin-test.sh
+++ b/tests/admin-test.sh
@@ -232,3 +232,19 @@ curr_rev=$(${CMD_PREFIX} ostree rev-parse --repo=sysroot/ostree/repo testos/buil
 assert_streq ${curr_rev} ${head_rev}
 
 echo "ok upgrade with and without override-commit"
+
+deployment=$(${CMD_PREFIX} ostree admin --sysroot=sysroot --print-current-dir)
+${CMD_PREFIX} ostree --repo=sysroot/ostree/repo --sysroot=sysroot remote add --set=gpg-verify=false remote-test-physical file://$(pwd)/testos-repo
+assert_not_has_file ${deployment}/etc/ostree/remotes.d/remote-test-physical.conf testos-repo
+assert_file_has_content sysroot/ostree/repo/config remote-test-physical
+echo "ok remote add physical sysroot"
+
+# Now a hack...symlink ${deployment}/sysroot to the sysroot in lieu of a bind
+# mount which we can't do in unit tests.
+ln -sr sysroot ${deployment}/sysroot
+ln -s sysroot/ostree ${deployment}/ostree
+ls -al ${deployment}
+${CMD_PREFIX} ostree --repo=sysroot/ostree/repo --sysroot=${deployment} remote add --set=gpg-verify=false remote-test-nonphysical file://$(pwd)/testos-repo
+assert_not_file_has_content sysroot/ostree/repo/config remote-test-nonphysical
+assert_file_has_content ${deployment}/etc/ostree/remotes.d/remote-test-nonphysical.conf testos-repo
+echo "ok remote add nonphysical sysroot"

diff --git a/tests/test-admin-deploy-grub2.sh b/tests/test-admin-deploy-grub2.sh
index 2b90c2866529e596524aa8352e9dd4955655646c..d7c1c6db595af7bb7dd4809445e8d337abc5ea5e 100755 (executable)
--- a/tests/test-admin-deploy-grub2.sh
+++ b/tests/test-admin-deploy-grub2.sh
@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..16"
+echo "1..18"
 
 . $(dirname $0)/libtest.sh
 

diff --git a/tests/test-admin-deploy-syslinux.sh b/tests/test-admin-deploy-syslinux.sh
index 70b3b4d3e67f20738185a3877cb32043241f6b76..797836f082027976e9a1b40de5cb8bde94eb619f 100755 (executable)
--- a/tests/test-admin-deploy-syslinux.sh
+++ b/tests/test-admin-deploy-syslinux.sh
@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..16"
+echo "1..18"
 
 . $(dirname $0)/libtest.sh
 

diff --git a/tests/test-admin-deploy-uboot.sh b/tests/test-admin-deploy-uboot.sh
index d4c3a0dbf9349889e6638fa627e398a4034f6ca5..d9104f8cb238faa9da5ad2f66ffbb01e4b00272c 100755 (executable)
--- a/tests/test-admin-deploy-uboot.sh
+++ b/tests/test-admin-deploy-uboot.sh
@@ -20,7 +20,7 @@
 
 set -euo pipefail
 
-echo "1..17"
+echo "1..19"
 
 . $(dirname $0)/libtest.sh